record transaction fees #10715
record transaction fees #10715
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
8846972
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://384258a6.agoric-sdk.pages.dev |
| Branch Preview URL: | https://10578-transaction-fees.agoric-sdk.pages.dev |
| zone.subZone('status-manager'), | ||
| t.context.txnsNode, | ||
| ); | ||
| const { statusManager } = t.context; |
There was a problem hiding this comment.
statusManager is not stateless, so sharing it across tests is surprising. Consider adding a comment to say that this is done on purpose and why.
There was a problem hiding this comment.
it's constructed in beforeEach so it's not shared across tests.
There was a problem hiding this comment.
ah. beforeEach. I missed that. cool.
|
|
||
| const capData = await E(marshaller).toCapData(record); | ||
|
|
||
| await E(txNode).setValue(JSON.stringify(capData)); |
There was a problem hiding this comment.
why await the setValue?
I'm fuzzy on our void vs await conventions.
There was a problem hiding this comment.
the function has to be async to get capData before stringifying it.
this final await is just to provide the option to the caller to await that part. All the consumers use void now but voiding here wouldn't change the need for that.
| @@ -164,7 +162,7 @@ export const prepareStatusManager = ( | |||
| publishEvidence(txHash, evidence); | |||
| if (status !== PendingTxStatus.Observed) { | |||
| // publishEvidence publishes Observed | |||
| publishStatus(txHash, status); | |||
| void publishTxnRecord(txHash, harden({ status })); | |||
There was a problem hiding this comment.
does void mean "I'm certain this never rejects" or "if it rejects, there's nothing useful I can do, so I might as well let standard unhandled rejection logging do its thing"?
| storedCompletedTxs.add(txId); | ||
| } | ||
|
|
||
| const capData = await E(marshaller).toCapData(record); |
There was a problem hiding this comment.
side note: This adds a round-trip to the board vat; I'm curious whether it has a noticeable / measurable performance impact. Here's hoping I/we measure that in...
| @@ -40,6 +41,7 @@ export interface CctpTxEvidence { | |||
| */ | |||
| export interface TransactionRecord extends CopyRecord { | |||
There was a problem hiding this comment.
This type could be more precise. It could be a union that requires evidence in OBSERVED and split in DISBURSED.
There was a problem hiding this comment.
could be. I went this way to maintain the option to include these any time.
turadg
force-pushed
the
10578-transaction-fees
branch
from
f27f72f to
8846972
Compare
closes: #10578
Description
Adds the fee
splitto the Disbursed status of TransactionRecord.Also updates tests to have a marshaller that is capable of marshaling brands in the split.
Security Considerations
Gives a read-only marshaller to StatusManager.
Scaling Considerations
a little more data recorded on disbursement. Since that's a terminal state it will be retained until the GC action is run in a core-eval (or we are able to delete upon completion)
Documentation Considerations
updated types
Testing Considerations
CI
Upgrade Considerations
not deployed